Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2342 - Snowflake S3 Stage #2354

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from

Conversation

stevenayers
Copy link

Description

This PR fixes the issue described in #2342. It also breaks out gen_copy_sql into it's own file and standalone function so it's easier to test.

Related Issues

Additional Context

Copy link

netlify bot commented Feb 26, 2025

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit 0029f1e
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/67ce7d3c65c95f000884a046

@stevenayers stevenayers force-pushed the fix/2342-snowflake-s3-stage branch 2 times, most recently from 70fed71 to 6fd1576 Compare February 27, 2025 08:14
@rudolfix rudolfix self-requested a review March 1, 2025 10:51
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR and extensive test suite! Your code looks good as well.
Please submit your changes against devel branch. Unfortunately this version contains changes in gen_copy_sql (vectorized scanner option) so you'll get conflicts with your current version.

@stevenayers stevenayers changed the base branch from master to devel March 2, 2025 15:58
@stevenayers
Copy link
Author

thanks for the PR and extensive test suite! Your code looks good as well. Please submit your changes against devel branch. Unfortunately this version contains changes in gen_copy_sql (vectorized scanner option) so you'll get conflicts with your current version.

@rudolfix done 👍

@stevenayers stevenayers requested a review from rudolfix March 3, 2025 09:47
@rudolfix rudolfix added the ci from fork run ci workflows on a pr even if they are from a fork label Mar 3, 2025
@stevenayers stevenayers force-pushed the fix/2342-snowflake-s3-stage branch from fbdd4aa to 69924e9 Compare March 4, 2025 13:26
@rudolfix rudolfix removed the ci from fork run ci workflows on a pr even if they are from a fork label Mar 9, 2025
@rudolfix rudolfix self-assigned this Mar 9, 2025
@rudolfix rudolfix added the ci from fork run ci workflows on a pr even if they are from a fork label Mar 9, 2025
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the update. IMO vectorized scanner config options is not passed. also please mark your tests as essential (so we always run them - they are fast).

if you are ok with it: could you type signatures of your tests and fixtures? cursor probably can do that automatically.
we are typing all our tests and plan to make mypy more strict soon...

thanks!

@stevenayers
Copy link
Author

thanks for the update. IMO vectorized scanner config options is not passed. also please mark your tests as essential (so we always run them - they are fast).

if you are ok with it: could you type signatures of your tests and fixtures? cursor probably can do that automatically.

we are typing all our tests and plan to make mypy more strict soon...

thanks!

Np on the tests... but I'm pretty sure use vectorized scanner is passed. There's a unit test that checks for it too.

@rudolfix
Copy link
Collaborator

rudolfix commented Mar 9, 2025

please take a look at those tests:

FAILED tests/load/pipeline/test_snowflake_pipeline.py::test_snowflake_use_vectorized_scanner[TRUE-snowflake-parquet-staging-credential-forwarding] - AttributeError: type object 'SnowflakeLoadJob' has no attribute 'gen_copy_sql'
FAILED tests/load/pipeline/test_snowflake_pipeline.py::test_snowflake_use_vectorized_scanner[FALSE-snowflake-parquet-staging-credential-forwarding] - AttributeError: type object 'SnowflakeLoadJob' has no attribute 'gen_copy_sql'

here's the deleted arg: https://github.com/dlt-hub/dlt/pull/2354/files#diff-f4f94b7bc9fa2649ffaa9cb51df285bca77b5ea826d5dc026ddfe6f61207c946L113
what we recently did: we enabled tests that require ci access to run form forks. after your last merge from devel they finally run.

thanks for working on this :)

@stevenayers stevenayers force-pushed the fix/2342-snowflake-s3-stage branch from c2cf509 to 0029f1e Compare March 10, 2025 05:48
@stevenayers
Copy link
Author

please take a look at those tests:

FAILED tests/load/pipeline/test_snowflake_pipeline.py::test_snowflake_use_vectorized_scanner[TRUE-snowflake-parquet-staging-credential-forwarding] - AttributeError: type object 'SnowflakeLoadJob' has no attribute 'gen_copy_sql'
FAILED tests/load/pipeline/test_snowflake_pipeline.py::test_snowflake_use_vectorized_scanner[FALSE-snowflake-parquet-staging-credential-forwarding] - AttributeError: type object 'SnowflakeLoadJob' has no attribute 'gen_copy_sql'

here's the deleted arg: https://github.com/dlt-hub/dlt/pull/2354/files#diff-f4f94b7bc9fa2649ffaa9cb51df285bca77b5ea826d5dc026ddfe6f61207c946L113 what we recently did: we enabled tests that require ci access to run form forks. after your last merge from devel they finally run.

thanks for working on this :)

Np! Sorry about that, it should work now. It's a pain that we can't emulate some of the destinations for local testing. I'm sure we could but it would be a real pain to set up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci from fork run ci workflows on a pr even if they are from a fork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snowflake COPY INTO cannot use Snowflake S3 Stage
2 participants